feat(channels): add Plivo SMS channel adapter - #20
Conversation
Fill the `sms` channel slot with a Plivo-backed adapter using Plivo's
Messaging API. Outbound sends via POST /v1/Account/{authId}/Message/;
inbound messages arrive on a webhook the host forwards to
handleIncomingWebhook, which verifies the X-Plivo-Signature-V3 signature
and fails closed before emitting.
Register the adapter in adapters/index.ts and document setup in
docs/features/CHANNELS.md. Unit tests cover outbound send, inbound
emit/drop, and a golden-fixture signature check against the Plivo SDK.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: sarveshpatil-plivo <sarvesh.patil@plivo.com>
Register the adapter under a dedicated 'plivo' ChannelPlatform instead of taking over the shared 'sms' slot, and restore the original Twilio entry in the channels doc (Plivo is added as a separate row/section, not a swap). Signed-off-by: sarveshpatil-plivo <sarvesh.patil@plivo.com>
- fail fast on 401/403 account-verification (auth is known-bad), while still tolerating transient/network errors at connect - log when an inbound webhook is dropped because the adapter is not connected Signed-off-by: sarveshpatil-plivo <sarvesh.patil@plivo.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Reviewer's GuideAdds a new Plivo-backed SMS messaging channel adapter, wires it into the existing channel registry and types, documents configuration and usage, and implements signature-verified inbound webhook handling with accompanying tests for outbound/inbound flows and Plivo V3 signature computation. Sequence diagram for inbound Plivo SMS webhook handlingsequenceDiagram
participant Plivo
participant HostApp
participant PlivoSmsChannelAdapter
participant AgentOSCore
Plivo->>HostApp: POST /plivo/inbound (SMS webhook)
HostApp->>PlivoSmsChannelAdapter: handleIncomingWebhook(body, meta)
PlivoSmsChannelAdapter->>PlivoSmsChannelAdapter: isFromPlivo(body, meta)
PlivoSmsChannelAdapter->>PlivoSmsChannelAdapter: computePlivoV3Signature(method, url, nonce, authToken, params)
alt valid_signature
PlivoSmsChannelAdapter->>AgentOSCore: emit({ type: message, platform: plivo, data: ChannelMessage })
else invalid_or_missing_signature
PlivoSmsChannelAdapter-->>AgentOSCore: [drop webhook]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughPlivo SMS support adds outbound messaging and inbound webhook handling. The adapter supports V3 and V2 signature verification, channel exports, platform typing, tests, and setup documentation. ChangesPlivo SMS channel
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Plivo
participant ExpressRoute
participant PlivoSmsChannelAdapter
participant AgentOS
Plivo->>ExpressRoute: POST signed SMS webhook
ExpressRoute->>PlivoSmsChannelAdapter: handleIncomingWebhook(body, method, URL, headers)
PlivoSmsChannelAdapter->>PlivoSmsChannelAdapter: Validate V3 or V2 signature
PlivoSmsChannelAdapter->>AgentOS: Emit normalized message event
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd Plivo SMS channel adapter with signed inbound webhook verification
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/io/channels/adapters/PlivoSmsChannelAdapter.ts" line_range="277-280" />
<code_context>
+ const headers = meta?.headers ?? {};
+ const signature = headerValue(headers, 'x-plivo-signature-v3');
+ const nonce = headerValue(headers, 'x-plivo-signature-v3-nonce');
+ const url = meta?.url ?? this.webhookUrl;
+ const method = (meta?.method ?? 'POST').toUpperCase();
+
+ if (!signature || !nonce || !url || !this.authToken) return false;
+
+ let expected: string;
</code_context>
<issue_to_address>
**suggestion:** Dropping inbound webhooks when `verifySignatureEnabled` is true but no URL is available may lead to silent misconfiguration.
Because `meta.url` may be missing and `this.webhookUrl` may be unset, all inbound messages are marked untrusted (`return false`) and then dropped, with no clear indication of misconfiguration. Consider either enforcing that `webhookUrl` is set in `doConnect` when `verifySignatureEnabled` is true, or logging a clear error when `url` is missing so operators understand why webhooks are being rejected.
Suggested implementation:
```typescript
const url = meta?.url ?? this.webhookUrl;
const method = (meta?.method ?? 'POST').toUpperCase();
if (!signature || !nonce || !this.authToken) return false;
if (!url) {
this.logger?.error(
'Plivo webhook signature verification failed: no request URL available. ' +
'Ensure webhookUrl is configured or meta.url is provided when verifySignatureEnabled is true.',
);
return false;
}
```
1. Ensure the adapter class has a `logger` instance (or similar) available; if not, introduce one consistent with the existing logging conventions in this project and pass it into `PlivoSmsChannelAdapter`.
2. Optionally, enforce that `webhookUrl` is set in your `doConnect` (or equivalent initialization) method when `verifySignatureEnabled` is true by validating configuration there and logging/throwing a configuration error early, so misconfiguration is caught before handling webhooks.
</issue_to_address>
### Comment 2
<location path="docs/features/CHANNELS.md" line_range="297" />
<code_context>
+```bash
+export PLIVO_AUTH_ID=your-auth-id
+export PLIVO_AUTH_TOKEN=your-auth-token
+export PLIVO_PHONE=+14150000000
+```
+
</code_context>
<issue_to_address>
**suggestion:** Consider clarifying the expected format of `PLIVO_PHONE` in the example.
Add a brief note indicating that `PLIVO_PHONE` should be in E.164 format (for example, `+14150000000`), or explicitly state the format Plivo requires to reduce the chance of misconfiguration.
</issue_to_address>
### Comment 3
<location path="src/io/channels/adapters/PlivoSmsChannelAdapter.ts" line_range="376" />
<code_context>
+// Plivo SMS Auth Params
+// ============================================================================
+
+/** Platform-specific parameters for a Plivo SMS connection. */
+export interface PlivoSmsAuthParams extends Record<string, string | undefined> {
+ /** Plivo Auth ID (account identifier). Required. */
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the adapter’s configuration and verification paths by tightening types, specializing POST-only webhook handling, and reducing generic helpers to the concrete Plivo SMS shapes you actually use.
You can trim some of the over‑generalization without losing any current behavior by tightening types and specializing the hot paths.
### 1. Use a boolean flag for signature verification
The current string protocol (`verifySignature !== 'false'`) is surprising. A boolean is simpler and self‑documenting:
```ts
// interface
export interface PlivoSmsAuthParams {
// ...
/** When false, inbound signature verification is disabled. Defaults to true. */
verifySignature?: boolean;
}
// doConnect
this.verifySignatureEnabled = params.verifySignature ?? true;
```
This keeps the default behavior (enabled) but removes the “anything but 'false'” quirk.
### 2. Make `PlivoSmsAuthParams` a closed interface
`extends Record<string, string | undefined>` suggests arbitrary keys, which aren’t used and adds surface area. A closed interface is easier to reason about:
```ts
export interface PlivoSmsAuthParams {
/** Plivo Auth ID (account identifier). Required. */
authId?: string;
/** Plivo Auth Token. If omitted, the `credential` field is used. */
authToken?: string;
/** Plivo sender number / short code / sender id used as `src`. Required. */
phoneNumber?: string;
/** Externally-visible message URL Plivo signs; used to verify inbound webhooks. */
webhookUrl?: string;
/** When false, inbound signature verification is disabled. Defaults to true. */
verifySignature?: boolean;
}
```
If you really need arbitrary extra keys, you can explicitly model them:
```ts
/** Extra provider-specific options. */
extraParams?: Record<string, string | undefined>;
```
### 3. Specialize the inbound verification path to POST callbacks
The adapter only ever verifies POST callbacks. You can simplify `isFromPlivo` by hardcoding POST instead of threading a generic `method`, and by requiring `url` when verification is on:
```ts
handleIncomingWebhook(
body: Record<string, unknown>,
meta?: {
/** The exact externally-visible URL Plivo POSTed to (must byte-match). */
url: string;
headers: Record<string, string | string[] | undefined>;
},
): void {
if (this.status !== 'connected') {
console.warn('[Plivo SMS] Dropping inbound webhook — adapter not connected.');
return;
}
if (this.verifySignatureEnabled && (!meta || !this.isFromPlivo(body, meta))) {
console.warn('[Plivo SMS] Dropping inbound webhook — signature missing or invalid.');
return;
}
// ...
}
private isFromPlivo(
body: Record<string, unknown>,
meta: { url: string; headers: Record<string, string | string[] | undefined> },
): boolean {
const signature = headerValue(meta.headers, 'x-plivo-signature-v3');
const nonce = headerValue(meta.headers, 'x-plivo-signature-v3-nonce');
if (!signature || !nonce || !this.authToken) return false;
let expected: string;
try {
expected = computePlivoV3Signature({
method: 'POST', // only POST used by this adapter
url: meta.url,
nonce,
authToken: this.authToken,
params: body,
});
} catch {
return false;
}
// ...
}
```
This removes the “optional method + fallback URL” branching from the adapter surface, while still allowing `computePlivoV3Signature` to support GET in case you need it elsewhere.
### 4. Narrow `sortedParamsString` to the flat body shape you actually see
Plivo’s inbound SMS payload is a flat form. You can simplify the serializer to a flat map of primitives and drop recursion/array sorting (keeping the function exported and name intact):
```ts
export function computePlivoV3Signature(input: {
method: string;
url: string;
nonce: string;
authToken: string;
params: Record<string, string | number | boolean | undefined>;
}): string {
// ...
if (isPost) {
signed += sortedParamsString(input.params) + '.' + nonce;
}
// ...
}
function sortedParamsString(params: Record<string, string | number | boolean | undefined>): string {
let out = '';
for (const key of Object.keys(params).sort()) {
const value = params[key];
if (value === undefined) continue;
out += key + String(value);
}
return out;
}
```
Call sites that currently pass `Record<string, unknown>` can be constrained at the boundary (e.g., by coercing to `Record<string, string | number | boolean | undefined>`), but the adapter’s concrete usage (flat inbound payload) remains unchanged and the implementation is much easier to follow.
### 5. Avoid generic header scanning in a tight loop
Since `isFromPlivo` only cares about two headers, normalizing header keys once can be clearer than a generic search per lookup:
```ts
private isFromPlivo(
body: Record<string, unknown>,
meta: { url: string; headers: Record<string, string | string[] | undefined> },
): boolean {
const headersLower = new Map<string, string>();
for (const [k, v] of Object.entries(meta.headers)) {
const value = Array.isArray(v) ? v[0] : v;
if (typeof value === 'string') {
headersLower.set(k.toLowerCase(), value);
}
}
const signature = headersLower.get('x-plivo-signature-v3');
const nonce = headersLower.get('x-plivo-signature-v3-nonce');
// ...
}
```
With this in place, you can either inline `headerValue` usage here or drop the helper entirely; either way, you avoid iterating over all headers for each lookup.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Plivo SMS Auth Params | ||
| // ============================================================================ | ||
|
|
||
| /** Platform-specific parameters for a Plivo SMS connection. */ |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the adapter’s configuration and verification paths by tightening types, specializing POST-only webhook handling, and reducing generic helpers to the concrete Plivo SMS shapes you actually use.
You can trim some of the over‑generalization without losing any current behavior by tightening types and specializing the hot paths.
1. Use a boolean flag for signature verification
The current string protocol (verifySignature !== 'false') is surprising. A boolean is simpler and self‑documenting:
// interface
export interface PlivoSmsAuthParams {
// ...
/** When false, inbound signature verification is disabled. Defaults to true. */
verifySignature?: boolean;
}
// doConnect
this.verifySignatureEnabled = params.verifySignature ?? true;This keeps the default behavior (enabled) but removes the “anything but 'false'” quirk.
2. Make PlivoSmsAuthParams a closed interface
extends Record<string, string | undefined> suggests arbitrary keys, which aren’t used and adds surface area. A closed interface is easier to reason about:
export interface PlivoSmsAuthParams {
/** Plivo Auth ID (account identifier). Required. */
authId?: string;
/** Plivo Auth Token. If omitted, the `credential` field is used. */
authToken?: string;
/** Plivo sender number / short code / sender id used as `src`. Required. */
phoneNumber?: string;
/** Externally-visible message URL Plivo signs; used to verify inbound webhooks. */
webhookUrl?: string;
/** When false, inbound signature verification is disabled. Defaults to true. */
verifySignature?: boolean;
}If you really need arbitrary extra keys, you can explicitly model them:
/** Extra provider-specific options. */
extraParams?: Record<string, string | undefined>;3. Specialize the inbound verification path to POST callbacks
The adapter only ever verifies POST callbacks. You can simplify isFromPlivo by hardcoding POST instead of threading a generic method, and by requiring url when verification is on:
handleIncomingWebhook(
body: Record<string, unknown>,
meta?: {
/** The exact externally-visible URL Plivo POSTed to (must byte-match). */
url: string;
headers: Record<string, string | string[] | undefined>;
},
): void {
if (this.status !== 'connected') {
console.warn('[Plivo SMS] Dropping inbound webhook — adapter not connected.');
return;
}
if (this.verifySignatureEnabled && (!meta || !this.isFromPlivo(body, meta))) {
console.warn('[Plivo SMS] Dropping inbound webhook — signature missing or invalid.');
return;
}
// ...
}
private isFromPlivo(
body: Record<string, unknown>,
meta: { url: string; headers: Record<string, string | string[] | undefined> },
): boolean {
const signature = headerValue(meta.headers, 'x-plivo-signature-v3');
const nonce = headerValue(meta.headers, 'x-plivo-signature-v3-nonce');
if (!signature || !nonce || !this.authToken) return false;
let expected: string;
try {
expected = computePlivoV3Signature({
method: 'POST', // only POST used by this adapter
url: meta.url,
nonce,
authToken: this.authToken,
params: body,
});
} catch {
return false;
}
// ...
}This removes the “optional method + fallback URL” branching from the adapter surface, while still allowing computePlivoV3Signature to support GET in case you need it elsewhere.
4. Narrow sortedParamsString to the flat body shape you actually see
Plivo’s inbound SMS payload is a flat form. You can simplify the serializer to a flat map of primitives and drop recursion/array sorting (keeping the function exported and name intact):
export function computePlivoV3Signature(input: {
method: string;
url: string;
nonce: string;
authToken: string;
params: Record<string, string | number | boolean | undefined>;
}): string {
// ...
if (isPost) {
signed += sortedParamsString(input.params) + '.' + nonce;
}
// ...
}
function sortedParamsString(params: Record<string, string | number | boolean | undefined>): string {
let out = '';
for (const key of Object.keys(params).sort()) {
const value = params[key];
if (value === undefined) continue;
out += key + String(value);
}
return out;
}Call sites that currently pass Record<string, unknown> can be constrained at the boundary (e.g., by coercing to Record<string, string | number | boolean | undefined>), but the adapter’s concrete usage (flat inbound payload) remains unchanged and the implementation is much easier to follow.
5. Avoid generic header scanning in a tight loop
Since isFromPlivo only cares about two headers, normalizing header keys once can be clearer than a generic search per lookup:
private isFromPlivo(
body: Record<string, unknown>,
meta: { url: string; headers: Record<string, string | string[] | undefined> },
): boolean {
const headersLower = new Map<string, string>();
for (const [k, v] of Object.entries(meta.headers)) {
const value = Array.isArray(v) ? v[0] : v;
if (typeof value === 'string') {
headersLower.set(k.toLowerCase(), value);
}
}
const signature = headersLower.get('x-plivo-signature-v3');
const nonce = headersLower.get('x-plivo-signature-v3-nonce');
// ...
}With this in place, you can either inline headerValue usage here or drop the helper entirely; either way, you avoid iterating over all headers for each lookup.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts (1)
95-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for connection-hardening behaviors called out in this PR.
The commit summary highlights two hardening behaviors — failing fast on invalid account credentials (401/403) and logging inbound webhooks dropped while disconnected — but neither has a dedicated test here. Worth adding:
- A
doConnect/initializetest wheremakeFetchreturnsstatus: 401for the account GET, assertinginitializerejects.- A
handleIncomingWebhooktest called beforeconnect()(or aftershutdown()), asserting no event is emitted.Want me to draft these two test cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts` around lines 95 - 216, The Plivo adapter tests lack coverage for credential failures during initialization and webhook handling while disconnected. Add a connection test using makeFetch to return 401 for the account GET and assert initialize rejects, then add a handleIncomingWebhook test before connect or after shutdown that asserts no message event is emitted and preserves the disconnected-drop behavior.src/io/channels/adapters/PlivoSmsChannelAdapter.ts (1)
118-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile fail-fast detection via
Error.messagestring matching.The 401/403 fail-fast (Line 135) is re-detected in the
catchblock viaerr.message.includes('Authentication failed')(Line 139). Editing either string independently (e.g., i18n, wording tweak) silently turns a hard-auth failure into a tolerated warning, defeating the fail-fast goal. Prefer a typed/sentinel error (custom error class or a property flag) instead of substring matching.♻️ Suggested refactor
+class PlivoAuthError extends Error {} + if (resp.status === 401 || resp.status === 403) { - throw new Error(`[Plivo SMS] Authentication failed (HTTP ${resp.status}) — check authId/authToken.`); + throw new PlivoAuthError(`[Plivo SMS] Authentication failed (HTTP ${resp.status}) — check authId/authToken.`); } console.warn(`[Plivo SMS] Account verification returned HTTP ${resp.status}.`); } catch (err) { - if (err instanceof Error && err.message.includes('Authentication failed')) { + if (err instanceof PlivoAuthError) { throw err; } console.warn(`[Plivo SMS] Account verification failed: ${err}`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts` around lines 118 - 143, Replace the authentication-failure message substring check in the verification method with a typed or sentinel error shared by the 401/403 branch and the catch block. Update the branch that throws on unauthorized responses and the catch handling so this error is rethrown reliably regardless of message wording, while other failures remain warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/CHANNELS.md`:
- Line 48: Use PLIVO_PHONE_NUMBER consistently in both documented locations:
update the Plivo required-variable table at docs/features/CHANNELS.md lines
48-48, and update the setup example’s export and read references at
docs/features/CHANNELS.md lines 295-309. No other documentation changes are
needed.
In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts`:
- Around line 119-122: Add request timeouts to both outbound Plivo calls in the
PlivoSmsChannelAdapter: the account-verification fetch and the outbound Message/
POST. Pass an appropriate AbortSignal.timeout value in each fetch request while
preserving the existing headers, authentication, and response handling.
- Around line 270-340: Update isFromPlivo to validate Plivo SMS webhooks using
the X-Plivo-Signature-V2 header and its corresponding V2 verification algorithm,
instead of requiring X-Plivo-Signature-V3 and nonce. Preserve the existing
fail-closed behavior when the signature, URL, or auth token is missing, and use
the appropriate V2 signature helper or implementation.
---
Nitpick comments:
In `@src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts`:
- Around line 95-216: The Plivo adapter tests lack coverage for credential
failures during initialization and webhook handling while disconnected. Add a
connection test using makeFetch to return 401 for the account GET and assert
initialize rejects, then add a handleIncomingWebhook test before connect or
after shutdown that asserts no message event is emitted and preserves the
disconnected-drop behavior.
In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts`:
- Around line 118-143: Replace the authentication-failure message substring
check in the verification method with a typed or sentinel error shared by the
401/403 branch and the catch block. Update the branch that throws on
unauthorized responses and the catch handling so this error is rethrown reliably
regardless of message wording, while other failures remain warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 62bf8496-20ba-4390-8d90-c73c8c91f4fc
📒 Files selected for processing (5)
docs/features/CHANNELS.mdsrc/io/channels/adapters/PlivoSmsChannelAdapter.tssrc/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.tssrc/io/channels/adapters/index.tssrc/io/channels/types.ts
- Re-export PlivoSmsChannelAdapter and PlivoSmsAuthParams from the channels barrel so the documented import from '@framers/agentos' resolves. - Use PLIVO_PHONE_NUMBER in the docs to match the configured secret name, and note the sender must be E.164. - Log a clear warning when an inbound webhook cannot be verified because no request URL is available, instead of a silent failure that looks like a bad signature.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/io/channels/index.ts`:
- Line 14: Update the Phase 4 comment in the adapter barrel to accurately state
“base class + 12 platform adapters,” unless the intended behavior is to export
14, in which case add the two missing adapter exports. Keep the comment and
exported adapter count consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ee1ab8ec-0808-4601-a81b-f46e531a2e3b
📒 Files selected for processing (3)
docs/features/CHANNELS.mdsrc/io/channels/adapters/PlivoSmsChannelAdapter.tssrc/io/channels/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/features/CHANNELS.md
- src/io/channels/adapters/PlivoSmsChannelAdapter.ts
| export type { GroupPolicyInput, GroupPolicyResult, GroupPolicyReason } from './group-policy.js'; | ||
|
|
||
| // Phase 4: Adapter implementations — base class + 13 platform adapters | ||
| // Phase 4: Adapter implementations — base class + 14 platform adapters |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the adapter count in the phase comment.
This barrel currently exports 12 platform adapters, not 14. Update the comment to base class + 12 platform adapters, or add the two missing adapter exports if 14 is the intended total.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/io/channels/index.ts` at line 14, Update the Phase 4 comment in the
adapter barrel to accurately state “base class + 12 platform adapters,” unless
the intended behavior is to export 14, in which case add the two missing adapter
exports. Keep the comment and exported adapter count consistent.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…outs, fix adapter count - isFromPlivo now accepts every signature family Plivo may send: MA-V3 and plain V3 (params-folded), plus MA-V2 and V2 (url+nonce). Inbound SMS is signed under X-Plivo-Signature-MA-V3, so validating only plain V3 rejected genuine inbound webhooks. Each candidate is still matched in constant time. - Add computePlivoV2Signature helper and tests (V2 golden + MA-V3/V2 inbound). - Add AbortSignal.timeout to the account-verify GET and the outbound Message POST. - Correct the channels barrel adapter-count comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts`:
- Around line 10-12: Correct the Plivo SMS signature documentation and test
naming: in src/io/channels/adapters/PlivoSmsChannelAdapter.ts lines 10-12,
identify V2 as the documented SMS signature family; in lines 272-281, describe
V3 handling as compatibility behavior rather than standard inbound SMS behavior;
and in src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts lines
190-203, rename the test to remove the claim that the V3 header is the real SMS
wire header.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c35693e-6ff8-4624-a906-3683d36036b3
📒 Files selected for processing (3)
src/io/channels/adapters/PlivoSmsChannelAdapter.tssrc/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.tssrc/io/channels/index.ts
| * which verifies Plivo's inbound-message signature before emitting. Inbound | ||
| * messaging is signed under `X-Plivo-Signature-MA-V3`; the plain V3 and the | ||
| * V2 family are accepted as well. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Plivo messaging signature validation X-Plivo-Signature-V2 X-Plivo-Signature-Ma-V2 SMS
💡 Result:
Plivo uses signature validation to ensure that incoming webhook requests originate from its platform and have not been tampered with [1][2]. For messaging (SMS) callbacks, Plivo currently utilizes the V2 signature validation scheme [1][3][4]. Core Concepts for V2 Signature Validation: - Required Headers: All messaging requests from Plivo include X-Plivo-Signature-V2, X-Plivo-Signature-Ma-V2, and X-Plivo-Signature-V2-Nonce [1][2][5]. - Validation Options: You may use either X-Plivo-Signature-V2 or X-Plivo-Signature-Ma-V2 to perform validation [1][3]. - X-Plivo-Signature-V2: Generated using the Auth Token of the specific account or subaccount associated with the request [1][3]. - X-Plivo-Signature-Ma-V2: Generated using the Auth Token of the main account [1][3]. - Validation Logic: - Algorithm: HMAC-SHA256 [1][5]. - Key: Your Plivo Auth Token (account or main, depending on the header used) [1][5]. - Message: Concatenation of the full Base URI and the X-Plivo-Signature-V2-Nonce (e.g., https://yourdomain.com/callback/nonce_value) [1][5]. - Implementation: Plivo provides built-in validate_signature helper functions within their official Server SDKs to simplify this process [1][5][3]. Note on V3: While V2 is used for messaging, Plivo has introduced V3 signature validation for its Voice platform [6][4]. V3 provides improved security by including the request's POST parameters in the signature calculation and is recommended for migration where applicable, though V2 remains the standard for messaging [6][4].
Citations:
- 1: https://plivo.com/docs/messaging/concepts/signature-validation
- 2: https://plivo.com/docs/messaging/concepts/callbacks
- 3: https://plivo.com/docs/messaging/xml/overview
- 4: https://www.plivo.com/blog/how-to-secure-plivo-webhook-urls-in-python/
- 5: https://support.plivo.com/hc/en-us/articles/38549637789337-How-can-I-validate-that-OTP-requests-and-callbacks-are-authentic
- 6: https://plivo.com/docs/voice/concepts/signature-validation
🌐 Web query:
Plivo Voice callback signature V3 X-Plivo-Signature-V3
💡 Result:
The X-Plivo-Signature-V3 header is a security feature used by Plivo to ensure that incoming voice callback webhook requests genuinely originate from the Plivo platform and have not been tampered with in transit [1][2]. It uses an HMAC-SHA256 signature validation mechanism, which is more secure than the deprecated V2 version [1]. Key Components for Validation: - Header X-Plivo-Signature-V3: Contains the HMAC-SHA256 signature provided by Plivo. If multiple signatures exist (e.g., from subaccounts), Plivo may send them as a comma-separated list [1][3]. - Header X-Plivo-Signature-V3-Nonce: Contains a unique, randomly generated nonce string for every request [1]. - Header X-Plivo-Signature-Ma-V3: An alternative signature generated using the main account's Auth Token [1][4]. How to Verify: To validate the signature, you must generate your own HMAC-SHA256 hash and compare it against the value received in the header [1]. The process generally follows these steps: 1. Construct the canonical request string: This typically involves taking the request URL, the HTTP method (GET/POST), and the POST parameters (for POST requests), and combining them in a specific format defined by Plivo [1][3]. 2. Append the nonce: A dot (.) followed by the nonce string from the X-Plivo-Signature-V3-Nonce header is appended to this string [1][3][5]. 3. Sign the string: Use your Plivo Auth Token as the key to sign this string using the HMAC-SHA256 hashing algorithm [1]. 4. Compare: Encode the resulting hash in Base64 and compare it with the signature(s) provided in the X-Plivo-Signature-V3 header [1][3]. Plivo highly recommends using their official server-side SDKs, which include built-in helper functions to handle this validation process automatically [1][4][6]. Attempting to manually construct the signature can lead to errors if the canonical string format (including URL and parameter sorting) is not handled exactly as Plivo expects [3][7]. If validation fails, it is standard practice to reject the request to prevent potential spoofing or unauthorized call control [4].
Citations:
- 1: https://plivo.com/docs/voice/concepts/signature-validation
- 2: https://plivo.com/docs/voice/concepts/callbacks
- 3: https://github.com/plivo/plivo-go/blob/master/utils.go
- 4: https://www.plivo.com/blog/how-to-secure-plivo-webhook-urls-in-python/
- 5: https://www.rubydoc.info/gems/plivo/4.63.3/Plivo/Utils
- 6: https://github.com/plivo/plivo-examples-java/blob/master/voice/validateSignature.java
- 7: https://stackoverflow.com/questions/70679607/plivo-v3-validation-not-able-to-verify-signature-using-plivo-sdk
Correct the SMS signature-family statements.
Plivo Messaging documents V2 signatures for SMS callbacks (X-Plivo-Signature-V2, X-Plivo-Signature-Ma-V2, and X-Plivo-Signature-V2-Nonce). V3 signatures apply to Plivo Voice callbacks, so do not describe MA-V3 as the real or primary SMS header.
src/io/channels/adapters/PlivoSmsChannelAdapter.ts#L10-L12: State that V2 is the documented SMS signature family.src/io/channels/adapters/PlivoSmsChannelAdapter.ts#L272-281: Label V3 support as compatibility behavior, not normal inbound SMS behavior.src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts#L190-203: Rename this test to remove the “real wire header for SMS” claim.
📍 Affects 2 files
src/io/channels/adapters/PlivoSmsChannelAdapter.ts#L10-L12(this comment)src/io/channels/adapters/PlivoSmsChannelAdapter.ts#L272-L281src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts#L190-L203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts` around lines 10 - 12,
Correct the Plivo SMS signature documentation and test naming: in
src/io/channels/adapters/PlivoSmsChannelAdapter.ts lines 10-12, identify V2 as
the documented SMS signature family; in lines 272-281, describe V3 handling as
compatibility behavior rather than standard inbound SMS behavior; and in
src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts lines 190-203,
rename the test to remove the claim that the V3 header is the real SMS wire
header.
|
Hi @jddunn, this adds a Plivo SMS channel adapter under src/io/channels/adapters/, following the existing adapter pattern (outbound send plus an inbound webhook with signature verification). I have addressed the automated review feedback, including inbound signature verification (it now accepts every signature family Plivo sends, since inbound SMS is signed under X-Plivo-Signature-MA-V3). Typecheck, lint, and the adapter tests all pass locally. Would you be able to take a look when you get a chance? Happy to adjust anything. |
1 participant
Summary
through another provider.
Plivo, registered as its own
plivochannel platform alongside the existingadapters.
subsystem, filling the SMS gap without touching voice.
so unsigned or invalid requests are dropped.
Checklist
Summary by Sourcery
Add a Plivo-backed SMS messaging channel adapter and wire it into the channels platform registry.
New Features:
plivomessaging platform.Documentation:
Tests:
Summary by CodeRabbit